Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add benchamark #77

Merged
merged 1 commit into from
Dec 5, 2018
Merged

Add benchamark #77

merged 1 commit into from
Dec 5, 2018

Conversation

PikachuEXE
Copy link
Contributor

My result:

→ ruby benchmark/main.rb
-- create_table(:blogs, {:force=>true})
   -> 0.0035s
-- create_table(:posts, {:force=>true})
   -> 0.0004s
-- create_table(:users, {:force=>true})
   -> 0.0003s
-- create_table(:addresses, {:force=>true})
   -> 0.0003s
-- create_table(:groups, {:force=>true})
   -> 0.0003s
-- create_table(:tags, {:force=>true})
   -> 0.0003s
-- create_table(:post_tags, {:force=>true})
   -> 0.0003s
                           user     system      total        real
AR eager loading:      0.017352   0.000509   0.017861 (  0.017874)
AR with goldiloader:   0.032490   0.000850   0.033340 (  0.033918)

jturkel
jturkel reviewed Nov 30, 2018
View reviewed changes
Copy link
Member

@jturkel jturkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PikachuEXE - Thanks for the contribution! Adding some benchmarks to this project is a great idea. Do you know of any good tools that would allow us to generate good benchmark reports both with and without the goldiloader gem loaded since the ActiveRecord monkey patching could affect performance even with explicit eager loads?

benchmark/main.rb Outdated
Post.create!(author: user_2, blog: blog_2)
end

Benchmark.bm(20) do |x|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use benchmark-ips to add benchmark warm up, generate results with better statistical significance and provide better reporting of the difference?

benchmark/main.rb Outdated
Benchmark.bm(20) do |x|
# Use AR's eager loading
x.report("AR eager loading: ") do
::Blog.all.includes(posts: :author).map do |blog|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an each rather than a map? It doesn't look like the code does anything with the result.

benchmark/main.rb Outdated
# Use AR's eager loading
x.report("AR eager loading: ") do
::Blog.all.includes(posts: :author).map do |blog|
blog.posts.to_a.each do |post|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it matters but this benchmark will generate less garbage if we drop the to_a and call blog.posts.each.

@PikachuEXE
Copy link
Contributor Author

Benchmark updated

→ ruby benchmark/main.rb
-- create_table(:blogs, {:force=>true})
   -> 0.0088s
-- create_table(:posts, {:force=>true})
   -> 0.0004s
-- create_table(:users, {:force=>true})
   -> 0.0006s
-- create_table(:addresses, {:force=>true})
   -> 0.0003s
-- create_table(:groups, {:force=>true})
   -> 0.0003s
-- create_table(:tags, {:force=>true})
   -> 0.0007s
-- create_table(:post_tags, {:force=>true})
   -> 0.0003s
Warming up --------------------------------------
  AR eager loading:      6.000  i/100ms
AR with goldiloader:
                         3.000  i/100ms
Calculating -------------------------------------
  AR eager loading:      68.463  (± 4.4%) i/s -    342.000  in   5.003666s
AR with goldiloader:
                         36.166  (± 5.5%) i/s -    183.000  in   5.076981s

Comparison:
  AR eager loading: :       68.5 i/s
AR with goldiloader: :       36.2 i/s - 1.89x  slower

jturkel
jturkel requested changes Dec 4, 2018
View reviewed changes
Copy link
Member

@jturkel jturkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @PikachuEXE. The benchmark results look a lot more useful using benchmark-ips. The Travis builds are failing due to some Rubocop violations: https://travis-ci.org/salsify/goldiloader/jobs/463169992. A few small comments and then we can get this merged.

benchmark/db/schema.rb
@@ -0,0 +1,120 @@
# frozen_string_literal: true

ActiveRecord::Schema.define(version: 0) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this was copied from spec/db/schema.rb. If we think benchmarks and correctness tests will use the same schemas can we share that file across both types of tests. Alternatively we think the schemas for benchmarks and correctness tests will diverge, can we slim down benchmark/db/schema.rb to just the schema elements needed by the current benchmarks.

benchmark/main.rb Outdated

# Use goldiloader
x.report("AR with goldiloader: ") do
::Blog.all.map do |blog|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This map should be an each to be consistent with the "AR eager loading" test.

jturkel
jturkel reviewed Dec 4, 2018
View reviewed changes
Copy link
Member

@jturkel jturkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These benchmarks are already proving useful :) It looks like the perf difference in the benchmarks is coming from Goldiloader::AssociationPatch#eager_loadable?. We can get all of the performance back by caching the eager loadable checks that are based on model class level information that doesn't change between model instances (i.e. the results of the checks that hit Goldiloader::AssociationInfo).

benchmark/main.rb Outdated
@@ -0,0 +1,53 @@
# frozen_string_literal: true

$:.push File.expand_path("lib", __dir__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be File.expand_path("../lib", __dir__) so it points at the project root directory.

@jturkel jturkel mentioned this pull request Dec 5, 2018
Merged
@PikachuEXE
Copy link
Contributor Author

All issues fixed including rubocop

jturkel
jturkel approved these changes Dec 5, 2018
View reviewed changes
Copy link
Member

@jturkel jturkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks for the contribution!

@jturkel jturkel merged commit 556b934 into salsify:master Dec 5, 2018
@PikachuEXE PikachuEXE deleted the add-benchmark branch December 6, 2018 01:38
@PikachuEXE
Copy link
Contributor Author

Not sure if there is a way to run benchmark in CI...
Or should we?

@jturkel
Copy link
Member

jturkel commented Dec 6, 2018

I'd love to have benchmarks running in CI that can automatically flag performance regressions but I haven't seen this work well in oversubscribed CI environments with lots of noisy neighbors. I'm curious if others have seen this work well in other projects.

This was referenced Dec 6, 2018
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jturkel jturkel jturkel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@PikachuEXE @jturkel